Skip to content

Run tests with latest liquid/c gem#596

Merged
fw42 merged 3 commits intomasterfrom
liquid_c_tests
Jun 8, 2015
Merged

Run tests with latest liquid/c gem#596
fw42 merged 3 commits intomasterfrom
liquid_c_tests

Conversation

@fw42
Copy link
Copy Markdown
Contributor

@fw42 fw42 commented Jun 5, 2015

Turns out #592 is actually broken if liquid/c is present.

This PR makes it so that we run tests twice, once with and once without liquid/c enabled.

(Tests in this PR are going to fail for now. Justin is working on a fix in liquid/c.)

@pushrax @trishume

@fw42 fw42 force-pushed the liquid_c_tests branch from 293cd8d to 1f0e819 Compare June 5, 2015 18:54
@pushrax
Copy link
Copy Markdown
Contributor

pushrax commented Jun 5, 2015

I'm not sure about using master always, it means that when you check out an old liquid version the tests may fail...

Should we lock the liquid-c SHA in the gemfile?

@pushrax
Copy link
Copy Markdown
Contributor

pushrax commented Jun 5, 2015

Also, it will fail on non-MRI rubies :(

@fw42
Copy link
Copy Markdown
Contributor Author

fw42 commented Jun 5, 2015

Hmm good points

@trishume
Copy link
Copy Markdown
Contributor

trishume commented Jun 5, 2015

nice 2x2 test matrix, quadruple error messages if you screw up something globally 😈

In terms of version issues, maybe gate it by an environment variable that is set in CI?

@fw42 fw42 force-pushed the liquid_c_tests branch from 1f0e819 to 95e39b4 Compare June 5, 2015 19:27
parkr
parkr reviewed Jun 5, 2015
View reviewed changes
Comment thread test/test_helper.rb Outdated
Copy link
Copy Markdown
Contributor

@parkr parkr Jun 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but $stderr.puts here?

@parkr
Copy link
Copy Markdown
Contributor

parkr commented Jun 5, 2015

👍

@fw42
Copy link
Copy Markdown
Contributor Author

fw42 commented Jun 6, 2015

How about instead of using master, we use the latest released version of the liquid-c gem?

@pushrax
Copy link
Copy Markdown
Contributor

pushrax commented Jun 6, 2015

@fw42 I think that still has the same problem, and also makes updating liquid and liquid-c in tandem difficult.

@fw42
Copy link
Copy Markdown
Contributor Author

fw42 commented Jun 6, 2015

Hm I don't like the idea of hardcoding a liquid/c ref/version, we will forget to bump that.

@fw42 fw42 force-pushed the liquid_c_tests branch from 95e39b4 to 0b2656d Compare June 8, 2015 18:09
@fw42 fw42 force-pushed the liquid_c_tests branch from 09db460 to 01420e8 Compare June 8, 2015 18:38
fw42 added a commit that referenced this pull request Jun 8, 2015
@fw42
Merge pull request #596 from Shopify/liquid_c_tests
504b6fb 
Run tests with latest liquid/c gem
@fw42 fw42 merged commit 504b6fb into master Jun 8, 2015
@fw42 fw42 deleted the liquid_c_tests branch June 8, 2015 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fw42 @pushrax @trishume @parkr